Conversation
tgr
left a comment
There was a problem hiding this comment.
This PR doesn't contain most of the changes. You can always make a PR from the master into a new branch (instead of the other way around) if you don't want to rewrite master's history.
|
@tgr Sir, I have updated the PR to contain all the changes. |
App/templates/App/index.html
Outdated
There was a problem hiding this comment.
Technically, that's not valid HTML (which is why Github shows it in red); special characters like & or " need to be encoded in attributes. So it should be {{x.user}}&diff. In practice, it works anyway and no one really cares about, so feel free to ignore :) just mentioning for sake of completeness.
App/templates/App/index.html
Outdated
There was a problem hiding this comment.
The Phabricator username is not typically the same as the wiki username.
There was a problem hiding this comment.
Alright. I added that link to augment to the information. Since I just got to know that phabricator and wiki usernames are not same, I'd rather remove this link. Thank you .
App/views.py
Outdated
There was a problem hiding this comment.
Handled the situation. 👍
App/views.py
Outdated
There was a problem hiding this comment.
The request might fail (e.g. the API is down and responds with HTTP 503), in which case this won't work.
There was a problem hiding this comment.
Ok. Will handle this, when response code is other than 200.
App/views.py
Outdated
There was a problem hiding this comment.
The API might respond with an error, in which case this key might not exist. (You can test with a username containing <.)
App/views.py
Outdated
There was a problem hiding this comment.
Or possibly the user was found but has no edits.
There was a problem hiding this comment.
okay , this was a loophole, will handle it 👍
|
@tgr I have made the required changes . Please review. |
App/views.py
Outdated
There was a problem hiding this comment.
There might be other reasons for getting an API. You probably want to show the user the error message returned by the API instead.
App/views.py
Outdated
There was a problem hiding this comment.
But now the username will look weird when you print it out :)
Best practice is to sanitize close to the input, escape close to the output. (Sanitize = reject invalid input. Here the API takes care of that so you don't have to. Escape = convert the variable to a safe format. "safe" depends on context (you escape differently for text in a HTML document, for an URL part, for a Javascript variable...) so it has to be done when you already know how you'll use it; ideally in the template. Most templating languages (including Django templates) support that via some special notation.
There was a problem hiding this comment.
I have tried to make the variable safe, by escaping &. Is It ok?
App/views.py
Outdated
There was a problem hiding this comment.
Sir so should I consider input containing "&" as invalid , not deal with trying correct the handle ? Please tell . Currently , if I enter a username ey bd&808 , it converts to bd808 and displays the result.
There was a problem hiding this comment.
bd&808 is a different (valid) username though so that would prevent that person from using your tool. The percent-encoding approach wasn't wrong, you just did it at the wrong place (also, no point in limiting it to just &). See my earlier comment - you should do escaping in the template, and use whatever tools the templating language provides.
There was a problem hiding this comment.
Sir, in django templating, escaping is done by default on. Now, I have used 'safe' filter in my html file , so the username will not be auto-escaped. Am I going right now ?
|
@tgr Please review this PR. |
App/templates/App/index.html
Outdated
There was a problem hiding this comment.
This turns off escaping. There is no reason to do that and it will result in some usernames looking wrong (if they contain a HTML entity). It could also be a security hole although here it probably won't be since MediaWiki disallows <> in usernames.
Default autoescaping works well when you just want to display text. Where it doesn't work is when you want use a variable in an URL or a Javascript variable or any other context where the escaping/encoding rules are different from that of HTML.
There was a problem hiding this comment.
True, I understood not this thing in the first instance. Marking it safe is not the solution. So I have handled this now, using urlencoding. Now, the code works for all possible usernames else displays the error msg.
|
@tgr Sir, Please review, sorry for delay. |
tgr
left a comment
There was a problem hiding this comment.
Looks quite nice! The edit links use the wrong title parameter, but MediaWiki ignores the title anyway when oldid is set, so it doesn't matter much.
App/views.py
Outdated
There was a problem hiding this comment.
It would be more readable to put the constant parameters here as well.
@tgr Please review